Repository navigation
fix: keep ordinary fields that share a name with a secret - #172
ChiragAgg5k wants to merge 1 commit into
Conversation
The parse-error fallback redacts by field name. 'code' is the MFA challenge code but also a country's or language's code, so it hid those too. Leave out names that are ordinary strings on another model, as the CLI now does (appwrite/sdk-generator#2222).
🟡 Tier B · Needs changes before merging
This change excludes SDK field names that also appear as ordinary string fields from name-based redaction of unparsed responses. It adds a country-code regression case to the Appwrite error-formatting test.
Fix with agent prompt### Issue 1
src/mcp_server_appwrite/redaction.py:131
**Keep MFA `code` redacted in untyped responses**
`_format_response_parse_error` uses `redact_payload` for every unhydrated response, so removing `code` here disables its masking globally, not just for Country. If an MFA challenge response fails SDK hydration, its password-formatted one-time code is serialized verbatim into the MCP error result and may be retained in the conversation transcript.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 2
Reviewed |
![appwrite[bot]](https://avatars.githubusercontent.com/in/325556?s=60&v=4){kind=small}
| secret.add(field.alias or name) | ||
| elif _text(field.annotation): | ||
| plain.add(field.alias or name) | ||
| return frozenset(secret - plain) |
There was a problem hiding this comment.
Keep MFA code redacted in untyped responses
_format_response_parse_error uses redact_payload for every unhydrated response, so removing code here disables its masking globally, not just for Country. If an MFA challenge response fails SDK hydration, its password-formatted one-time code is serialized verbatim into the MCP error result and may be retained in the conversation transcript.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/mcp_server_appwrite/redaction.py
Line: 131
Comment:
**Keep MFA `code` redacted in untyped responses**
`_format_response_parse_error` uses `redact_payload` for every unhydrated response, so removing `code` here disables its masking globally, not just for Country. If an MFA challenge response fails SDK hydration, its password-formatted one-time code is serialized verbatim into the MCP error result and may be retained in the conversation transcript.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟠 Major · security · Reply if this doesn't apply.
|
Closing: the review is right. The by-name fallback only runs when the SDK can't parse a response, and over-masking a country's |
What does this PR do?
Follow-up to #170, the same fix as appwrite/sdk-generator#2222 for the CLI.
When the SDK can't parse a response, the error returns the raw payload, and #170 redacts it by field name: every name an
appwrite_consolemodel marksformat: password. That set includescode, fromMFAChallengeSecret.code, the one-time MFA code. So an unparsed country, language or locale payload would have its ordinarycodehidden._secret_namesnow leaves out any name that is also a non-secret string (or a list or optional of one) on another SDK model. Againstappwrite-console0.16.0, that drops onlycode. The other 30 names stay, includingsecret,passwordanduri.Typed results aren't affected: they're redacted per model field, so
MFAChallengeSecret.codeis still redacted there.Test Plan
test_format_appwrite_error_returns_the_unhydrated_payloadwith an unparsed country payload;"code": "DE"comes back untouched.ruff,black --checkandpyright(0 errors) pass.